feat: Account for custom S3 compatible storages (add optional parameters to AWS adapter) #11
Conversation
Louspirit
force-pushed
the
tobemerged
branch
from
207cf06
to
45170a3
Compare
V4 Signature for AWS Custom ServiceURL and Region for S3 compatible storage other than AWS Added and used some parameters Update AWSSDK.S3 to latest version possible (see aws/aws-sdk-net#2540 (comment))
Louspirit
force-pushed
the
tobemerged
branch
from
45170a3
to
6474ae1
Compare
|
I just forced push a correction. It should be ready for merge @alanedwardes. Please check that the required null meme type is also compatible for AWS S3. |
| Headers = new Dictionary<string, string> | ||
| { | ||
| {"Content-Type", BlobConstants.UploadMimeType} | ||
| } |
There was a problem hiding this comment.
The S3 supports headers, if the API in use does not support headers, it's not S3 compatible
There was a problem hiding this comment.
It does support headers, but it's UploadMimeType that breaks it.
So I was asking if we could remove it if not necessary for AWS.
Why are use using it in the first place? I found this example that's use content type Host like me, even with AWS.
This small PR would open to many cloud providers which are compatible with S3. I'm sure the community will understand you give support mostly for AWS, and they'll contribute if other cloud providers bring problems (like I did).
There was a problem hiding this comment.
You said you couldn't test it, but maybe you could test it on AWS S3.
It does not make sense in actual use as the default config would suffice, but it can assure you it works 👍🏻.
Something like:
S3_SERVICE_URL = "https://s3.eu-west-1.amazonaws.com"
S3Region = "eu-west-1";
S3AccessKey = "XXX";
S3AccessSecret = "XXX";
There was a problem hiding this comment.
Another use case that could be useful for people with this. It allows to use an AWS S3 bucket that's outside the stack, even in another region 😉
There was a problem hiding this comment.
Hello @alanedwardes, have you been able to try testing as I suggested?
| const string LfsBucket = "estranged-lfs-test"; | ||
| const string S3AccessKeyId = ""; | ||
| const string S3AccessKeySecret = ""; | ||
| const string S3Region = ""; | ||
| const string S3ServiceURL = ""; | ||
| if (!string.IsNullOrWhiteSpace(S3ServiceURL) && !string.IsNullOrWhiteSpace(S3Region) && !string.IsNullOrWhiteSpace(S3AccessKeyId) && !string.IsNullOrWhiteSpace(S3AccessKeySecret)) | ||
| { | ||
| services.AddLfsS3Adapter(new S3BlobAdapterConfig { Bucket = LfsBucket }, new AmazonS3Client(S3AccessKeyId, S3AccessKeySecret, new AmazonS3Config { ServiceURL = S3ServiceURL, AuthenticationRegion = S3Region, SignatureVersion = "V4" })); | ||
| } | ||
| else | ||
| { | ||
| services.AddLfsS3Adapter(new S3BlobAdapterConfig { Bucket = LfsBucket }, new AmazonS3Client()); | ||
| } |
There was a problem hiding this comment.
The point of these being packages on NuGet is so that this kind of granular customisation can happen outside of this repository (e.g. in your own application). I think this is too application specific to be useful in the sample application.
There was a problem hiding this comment.
You are just using the default values, which only restrict to AWS S3.
It opens the configuration with optional parameters without breaking the existing AWS one.
With this solution, anyone could just clone the repo and is ready to go (required only to change the parameters to fit its cloud provider).
AWSSDK.S3 has been fix
AWS fix the bug I reported. I updated to the latest SDK version. |
|
This is nice. One feature I would have liked to have was the ability to use S3 compatible storage providers, such as Cloudflare R2. |
V4 Signature for AWS
Custom ServiceURL and Region for S3 compatible storage other than AWS (OVH, Google, MinIO, Scaleway etc.)
Added and used some parameters
Update AWSSDK.S3 to the latest version possible (see aws/aws-sdk-net#2540 (comment))
Updated Readme with more details